Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/eslint newline error in model generator #2812

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

coolsoftwaretyler
Copy link
Contributor

@coolsoftwaretyler coolsoftwaretyler commented Oct 17, 2024

Please verify the following:

  • yarn test jest tests pass with new tests, if relevant
  • yarn lint eslint checks pass with new code, if relevant
  • yarn format:check prettier checks pass with new code, if relevant
  • README.md (or relevant documentation) has been updated with your changes
  • If this affects functionality there aren't tests for, I manually tested it, including by generating a new app locally if needed (see docs).

Describe your PR

In Ignite X, the model generator would create files that did not pass Prettier config. See "before" screenshot, which came from npx ignite-cli generate model Pizza in a fresh Ignite X app.

This change primarily fixes the prettier error, as seen in the "after" screenshot, which I got by running:

# In my local Ignite X repo
node ~/ignite/bin/ignite generate model --update
node ~/ignite/bin/ignite generate model FixedPizza

However, when I attempted to update the Jest snapshots, I ran into this issue from Jest: jestjs/jest#14305

One workaround is to disable prettier in Jest, which you'll see in cb78066. We could drop that commit and just take the snapshot changes from 8984194. I'm not sure if there's some other workaround or configuration in place that fixes that problem.

Screenshots (if applicable)

Before After
2024-10-17-eslint-error-with-missing-newline 2024-10-17-missing-newline-fixed

package.json Outdated
@@ -159,6 +159,7 @@
"tsx",
"js",
"jsx"
]
],
"prettierPath": null
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Workaround for jestjs/jest#14305

Comment on lines -60 to 62
"import { Instance, SnapshotIn, SnapshotOut, types } from \\"mobx-state-tree\\"
import { withSetPropAction } from \\"./helpers/withSetPropAction\\"
"import { Instance, SnapshotIn, SnapshotOut, types } from \\"mobx-state-tree\\"
import { withSetPropAction } from \\"./helpers/withSetPropAction\\"

Copy link
Contributor Author

@coolsoftwaretyler coolsoftwaretyler Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would guess this whitespace diff (and a few below) came from disabling Prettier in Jest, so maybe we should find a different way to fix that, or else it will cause a lot more noisy churn after the fact with other inline snapshots.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another approach would be ec682f8, which uses the workaround of a separate Prettier version for Jest. I set prettier-2 to be npm:[email protected] which I saw was the last version before the update in https://github.com/infinitered/ignite/pull/2761/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519.

Didn't seem to change the whitespace diff here, so maybe there's no real issue. Figured I'd at least provide the commit and we could easily drop it if the prettierPath: null is acceptable.

Comment on lines +78 to +79
// @mst remove-file
"
Copy link
Contributor Author

@coolsoftwaretyler coolsoftwaretyler Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the key snapshot diff that we do want to preserve: newline at end of file

@lindboe lindboe self-requested a review October 18, 2024 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant